Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

18068 - Namerequest UI: Editing the Resubmit NR has the first name choice as empty. #740

Merged
merged 5 commits into from
Oct 31, 2023

Conversation

Rajandeep98
Copy link
Contributor

@Rajandeep98 Rajandeep98 commented Oct 26, 2023

*Issue #18068 bcgov/entity#18068

Description of changes:
Updated condition for applying designation for resubmission for SP/GP

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM see new comment below

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing my approval -- I didn't looks closely enough at the code. The code change will always be True because it's not comparing the NR's type with an enum; it's looking at the truthiness of the enums directly, which is always True.

Rajandeep and I looked at this together and we're not sure why this code exists -- can't the designation from the original NR simply be copied in to the new NR? This code suggests that some NRs have a designation in the name but the designation field is empty. It would be good to have an example. We could then handle those specially.

The other option of exempting some types that we know don't have designations is less optimal but is probably safe and could be implemented as a last resort.

@Rajandeep98
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-740-qail5x3t.web.app

@bcgov bcgov deleted a comment from Rajandeep98 Oct 27, 2023
@bcgov bcgov deleted a comment from bcregistry-sre Oct 27, 2023
Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve this solution.

@severinbeauvais
Copy link
Collaborator

Do not merge this until after the release on Tuesday, October 31.

@Rajandeep98
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-740-qail5x3t.web.app

Copy link
Collaborator

@ozamani9gh ozamani9gh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@Rajandeep98
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-740-qail5x3t.web.app

@ozamani9gh ozamani9gh merged commit ba0d5fa into bcgov:main Oct 31, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants